Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding log sort flag to disable stable sort and get better memory performance in some situations #17354

Closed
wants to merge 15 commits into from

Conversation

YuanHao97
Copy link
Contributor

fix #17111
add log sorting flag to disable stable sort and get better memory performance in some situations.
command line flag: --execution_log_sort
possible values:
true : use stable sort as before when writing logs to a file. See com.google.devtools.build.lib.bazel.execlog.StableSort#stableSort
false : do not sort logs. Write directly from input stream to output stream.

@google-cla
Copy link

google-cla bot commented Jan 29, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@sgowroji sgowroji added team-Performance Issues for Performance teams awaiting-user-response Awaiting a response from the author awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Jan 29, 2023
@YuanHao97
Copy link
Contributor Author

@tjgq Hi, I've updated some files and got 5 failed tests. How can I resolve those failed checks? Thanks!

@tjgq
Copy link
Contributor

tjgq commented Feb 8, 2023

@tjgq Hi, I've updated some files and got 5 failed tests. How can I resolve those failed checks? Thanks!

Please ignore the failing tests on MacOS for now; they're unrelated to your PR. Someone else is working on fixing them.

@tjgq tjgq self-assigned this Feb 8, 2023
@tjgq tjgq added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Feb 8, 2023
@tjgq
Copy link
Contributor

tjgq commented Feb 8, 2023

@YuanHao97 at least one of the failures seems to be caused by your PR:

src/main/java/com/google/devtools/build/lib/bazel/SpawnLogModule.java:190: error: could not resolve MessageOutputStream
--
  | private void ignoreSort(InputStream in, MessageOutputStream out) throws IOException {
  | ^

In addition, please do the following before this PR can be merged:

  • Sign the CLA
  • Accept my suggested edit to the flag description in ExecutionOptions.java (if you agree with it)

I think the other broken tests have now been fixed, so you should get a cleaner result when you push again.

@YuanHao97
Copy link
Contributor Author

@tjgq Sorry, I got some compiling problems before push, so it's difficult to do verification.
BTW, I'm using Intellij IDEA and seems some packages are not found:
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.exec.Protos.SpawnExec;
I'm not sure why com.google.common.collect.ImmutableList is not found. Is there any more dependencies I should add?
I guess com.google.devtools.build.lib.exec.Protos.SpawnExec should be generated by some protobuf plugins, at least for maven project. But how to do that in Bazel project?

@tjgq
Copy link
Contributor

tjgq commented Feb 10, 2023

@tjgq Sorry, I got some compiling problems before push, so it's difficult to do verification. BTW, I'm using Intellij IDEA and seems some packages are not found: import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.exec.Protos.SpawnExec; I'm not sure why com.google.common.collect.ImmutableList is not found. Is there any more dependencies I should add? I guess com.google.devtools.build.lib.exec.Protos.SpawnExec should be generated by some protobuf plugins, at least for maven project. But how to do that in Bazel project?

To get Intellij support for Bazel builds, see https://ij.bazel.build/.

I think this looks good now. Could you please sign the CLA (#17354 (comment)) so I can start the import process?

@YuanHao97
Copy link
Contributor Author

@tjgq I've already siged cla. Please take a look at the check list.

@sgowroji sgowroji added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Feb 11, 2023
@tjgq tjgq added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Feb 15, 2023
@tjgq
Copy link
Contributor

tjgq commented Feb 15, 2023

@tjgq I've already siged cla. Please take a look at the check list.

My bad, sorry. I'll start the import process.

@brentleyjones
Copy link
Contributor

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Feb 15, 2023
@ShreeM01
Copy link
Contributor

@bazel-io fork 6.1.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Feb 15, 2023
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Feb 16, 2023
ShreeM01 added a commit that referenced this pull request Feb 16, 2023
This may improve performance when the execution log gets very large. The default is still to sort, so this is a backwards-compatible change.

Closes #17354.
Closes #17111.

PiperOrigin-RevId: 509822315
Change-Id: If948ec4a933389b6f8405985813dd76c549c445c

Co-authored-by: Hao Yuan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Performance Issues for Performance teams
Projects
None yet
6 participants